Post-merge-review: Fix template-no-action-modifiers autofix: skip when hash pairs are present#2654
Merged
NullVoxPopuli merged 3 commits intoember-cli:masterfrom Apr 14, 2026
Conversation
The autofix replaced only the path and positional params, leaving hash
pairs like on="click" behind. This produced broken output such as
{{on "click" this.handleClick on="click"}}. Suppress autofix whenever
hash pairs exist to avoid emitting invalid code.
| }, | ||
| { | ||
| // Path expression with hash pair (on="click") — no autofix to avoid leaving behind stale hash | ||
| code: '<template><button {{action this.handleClick on="click"}}>Save</button></template>', |
Contributor
There was a problem hiding this comment.
we know what to do here tho, we can autofix
Contributor
Author
There was a problem hiding this comment.
okay, thanks, tried a fix now
When the modifier has a path-expression handler and the only hash pair is
`on="<string>"`, we now read the event name from that pair and produce
`{{on "<event>" handler}}`, dropping the hash pair in the output.
Non-`on` hash pairs still suppress the fix since they have no clean
mapping to the `on` modifier's argument list.
NullVoxPopuli
approved these changes
Apr 14, 2026
This was referenced Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's broken on
masterThe autofix uses
fixer.replaceTextRange([node.path.range[0], lastParam.range[1]], ...)which only replaces text fromactionthrough the last positional param. Hash pairs (on="click",preventDefault=false, etc.) appear after the last positional param and are left in place, producing broken output:Fix
Disable autofix when any hash pair is present on the
{{action}}modifier. Detection still reports the error.Why not a smarter autofix
Upstream's fix uses
ember-template-recastAST mutation (no-action-modifiers.jsL55–61) and doesn't translateon=hash into{{on "eventName"}}either — so it would inherit the same issue. A smarter fix (convertingon="submit"into the first arg to{{on}}) is possible but out of scope for a regression fix.Test plan
{{action this.handleClick on="click"}},{{action this.handleSubmit on="submit"}}) fail on master with the exact broken output shown aboveCo-written by Claude.